Conversation
extra/interface/ap/build.gradle
Outdated
|
|
||
| tasks.withType(Test).configureEach { | ||
| // See: https://github.com/google/compile-testing/issues/222 | ||
| jvmArgs '--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED' |
There was a problem hiding this comment.
Locally you just test with whatever your current JVM runtime is -- this fails on CI because we execute tests on a bunch of different JVMs, including J8 where these module args don't exist. You can simulate this conditional locally with the -PstrictMultireleaseVersions=true gradle property (or just setting the CI environment variable).
# Conflicts: # .gitignore # gradle/libs.versions.toml
J8 still fails to build locally, but with a different issue
zml2008
left a comment
zml2008
left a comment
There was a problem hiding this comment.
Thanks for this PR! I like the concept for interface configs, but please see my comments for some design and implementation questions I have.p
| */ | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| @Target({ElementType.METHOD, ElementType.FIELD}) | ||
| public @interface DefaultBoolean { |
There was a problem hiding this comment.
I feel like annotations are too limited to properly express defaults -- maybe it'd make sense to do something likes immutables' defaultIsDefault option (or a similar per-method annotation), which would source the default value from the default implementation of the interface.
There was a problem hiding this comment.
I was planning to add that, but instead of adding an annotation to it it'd automatically use the default value.
The idea behind it is that in a config class/interface you expect config nodes, and if there are methods that aren't related to config nodes you @Exclude them. Does that also fit your vision?
| */ | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| @Target(ElementType.METHOD) | ||
| public @interface NumericRange { |
There was a problem hiding this comment.
These range annotations shouldn't be part of the interface extra module -- if we have these sorts of things, they should be constraints in core (and as a separate PR)
|
|
||
| @Override | ||
| public Set<String> getSupportedAnnotationTypes() { | ||
| return Collections.singleton(ConfigSerializable.class.getCanonicalName()); |
There was a problem hiding this comment.
ideally we can do this without putting configurate itself on the processor path maybe?
...n/java/org/spongepowered/configurate/interfaces/processor/ConfigImplementationGenerator.java
Outdated
Show resolved
Hide resolved
...n/java/org/spongepowered/configurate/interfaces/processor/ConfigImplementationGenerator.java
Outdated
Show resolved
Hide resolved
...n/java/org/spongepowered/configurate/interfaces/processor/ConfigImplementationGenerator.java
Outdated
Show resolved
Hide resolved
...g/spongepowered/configurate/interfaces/processor/ConfigImplementationGeneratorProcessor.java
Show resolved
Hide resolved
| final @Nullable Object obj, | ||
| final ConfigurationNode node | ||
| ) throws SerializationException { | ||
| throw new SerializationException( |
There was a problem hiding this comment.
This feels like it'll cause issues, I think you have to delegate to the impl class here too.
| * @return the default ConfigurationOptions with {@link InterfaceTypeSerializer} added to the serializers. | ||
| * @since 4.2.0 | ||
| */ | ||
| public static ConfigurationOptions get() { |
There was a problem hiding this comment.
It's better to expose just the TSC rather than a whole ConfigurationOptions instance. YAML users who use their own options instance already have issues as-is cuz snakeyaml has its own serialization logic.
Let's say interface Y extends interface X and method 'a' is defined in X. If you use enclosed element on 'a' you'll always get X (even for Y). superinterface is the interface where an impl is generated for.
3 participants
At the moment it's just a draft as I don't have everything I want to implement implemented yet, but gives a general idea of how I intent things to work.
I'll update the description along the way.